fix: refactor clear icon to button element to make it accessible#1224
Conversation
|
@Pareder is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
概览PR 为 Select 组件的清除按钮增加 变更内容Clear 按钮可访问性标签支持
可能相关的 PR
建议的审查人
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/SelectInput/index.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/hooks/useAllowClear.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. tests/Select.test.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1224 +/- ##
=======================================
Coverage 99.44% 99.44%
=======================================
Files 31 31
Lines 1270 1271 +1
Branches 465 466 +1
=======================================
+ Hits 1263 1264 +1
Misses 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request improves the accessibility and keyboard navigation of the clear button in the Select component by changing its markup from a div to a native button element and adding support for an accessible label. Feedback on these changes suggests adding a default fallback for the label to prevent type contract violations and ensure accessibility. Additionally, minor cleanups are recommended in the tests to remove an unused mock and simplify click event dispatching.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return { | ||
| allowClear: mergedAllowClear, | ||
| clearIcon: mergedAllowClear ? allowClearConfig.clearIcon || clearIcon || '×' : null, | ||
| label: mergedAllowClear ? allowClearConfig.label : '', |
There was a problem hiding this comment.
The AllowClearConfig interface defines label as a required string. However, when allowClear is passed as true or as an object without a label property, allowClearConfig.label will be undefined. Returning allowClearConfig.label directly violates the type contract and leaves the clear button without an accessible name for screen readers.\n\nProviding a default fallback like 'clear' resolves the type mismatch and ensures the button remains accessible by default.
| label: mergedAllowClear ? allowClearConfig.label : '', | |
| label: mergedAllowClear ? allowClearConfig.label || 'clear' : '', |
| const clickPreventDefault = jest.fn(); | ||
| const { container } = render( |
There was a problem hiding this comment.
| const clickEvent = createEvent.click(container.querySelector('.rc-select-clear')); | ||
| clickEvent.preventDefault = clickPreventDefault; | ||
| fireEvent(container.querySelector('.rc-select-clear'), clickEvent); |
There was a problem hiding this comment.
Since we don't need to mock preventDefault on the click event, we can simplify the event dispatching by directly calling fireEvent.click.
| const clickEvent = createEvent.click(container.querySelector('.rc-select-clear')); | |
| clickEvent.preventDefault = clickPreventDefault; | |
| fireEvent(container.querySelector('.rc-select-clear'), clickEvent); | |
| fireEvent.click(container.querySelector('.rc-select-clear')); |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/useAllowClear.tsx`:
- Line 41: The clear button label can be undefined when callers pass allowClear
or clearIcon without allowClear.label; update useAllowClear to provide a
non-empty fallback string for the button label (e.g. a default like "Clear" or
an i18n key) instead of passing allowClearConfig.label directly—locate the
mergedAllowClear construction and the place where label: mergedAllowClear ?
allowClearConfig.label : '' is set and replace it so label is always a safe,
non-empty string (refer to mergedAllowClear and allowClearConfig.label in
useAllowClear).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e63c184-4f12-49e8-b5e1-a7e46891d276
⛔ Files ignored due to path filters (1)
tests/__snapshots__/Select.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/BaseSelect/index.tsxsrc/SelectInput/index.tsxsrc/hooks/useAllowClear.tsxtests/Select.test.tsxtests/shared/allowClearTest.tsx
|
@afc163 @zombieJ @meet-student @QDyanbing Could you please review? |
1 similar comment
|
@afc163 @zombieJ @meet-student @QDyanbing Could you please review? |
|
Thanks for working on this. This is the right lower-level change for ant-design/ant-design#58267: Ant Design can pass the localized One thing still needs to be fixed here before this is ready: Could you update it to something like: label: mergedAllowClear ? allowClearConfig.label ?? 'Clear' : '',The existing button/click behavior and tests look aligned with what Ant Design needs. After this lands and is released, we can bump |
|
@afc163 Done. |
This PR improves clear button functionality by:
divto navitebuttonelementaria-labelattribute by extendingallowClearobject typeSummary by CodeRabbit
发行说明
allowClear现在可同时提供自定义清除按钮标签(用于无障碍展示)。button,支持键盘触发;自动补全并传递清除按钮的无障碍aria-label。